Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Deleting a SharedAttribute does not validate if it's possible to send the Notification #398

Merged
merged 30 commits into from
Feb 10, 2025
Merged

Deleting a SharedAttribute does not validate if it's possible to send the Notification #398

merged 30 commits into from
Feb 10, 2025

Conversation

sebbi08
Copy link
Contributor

@sebbi08 sebbi08 commented Jan 27, 2025 •
edited
Loading

Readiness checklist

  • I added/updated tests.
  • I ensured that the PR title is good enough for the changelog.
  • I labeled the PR.
  • I self-reviewed the PR.

@sebbi08 sebbi08 added the wip Work in Progress (blocks mergify from auto update the branch) label Jan 27, 2025
@codecov Codecov
Copy link

codecov bot commented Jan 27, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 84.21053% with 9 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...eteThirdPartyRelationshipAttributeAndNotifyPeer.ts 75.00% 4 Missing ⚠️
...ttributes/DeleteOwnSharedAttributeAndNotifyPeer.ts 84.61% 2 Missing ⚠️
...ributes/DeletePeerSharedAttributeAndNotifyOwner.ts 85.71% 2 Missing ⚠️
...ransport/src/modules/messages/MessageController.ts 85.71% 1 Missing ⚠️
Files with missing lines Coverage Δ
...kages/runtime/src/useCases/common/RuntimeErrors.ts 84.25% <100.00%> (+0.32%) ⬆️
...ransport/src/modules/messages/MessageController.ts 87.79% <85.71%> (ø)
...ttributes/DeleteOwnSharedAttributeAndNotifyPeer.ts 92.10% <84.61%> (-1.75%) ⬇️
...ributes/DeletePeerSharedAttributeAndNotifyOwner.ts 92.20% <85.71%> (-1.64%) ⬇️
...eteThirdPartyRelationshipAttributeAndNotifyPeer.ts 92.68% <75.00%> (-4.38%) ⬇️

... and 2 files with indirect coverage changes

@jkoenig134
Copy link
Member

with #404 this can be defenitely closed as we can just send these messages right away now

@sebbi08 sebbi08 changed the title Revoke relationship in status pending when the neded Attribute is deleted Return an error when deleting a SharedAttribute where the Relationship is still pending Jan 31, 2025
@sebbi08 sebbi08 marked this pull request as ready for review January 31, 2025 10:42
@sebbi08 sebbi08 added bug Something isn't working and removed wip Work in Progress (blocks mergify from auto update the branch) labels Jan 31, 2025
@sebbi08
Copy link
Contributor Author

sebbi08 commented Jan 31, 2025

with #404 this can be defenitely closed as we can just send these messages right away now

Sadly, the messages cannot be encrypted until the relationship got accepted by both Peers

@jkoenig134
Copy link
Member

image
?

britsta
britsta reviewed Jan 31, 2025
View reviewed changes
packages/runtime/src/useCases/common/RuntimeErrors.ts Outdated Show resolved Hide resolved
packages/runtime/src/useCases/common/RuntimeErrors.ts Outdated Show resolved Hide resolved
packages/runtime/src/useCases/consumption/attributes/DeleteOwnSharedAttributeAndNotifyPeer.ts Outdated Show resolved Hide resolved
packages/runtime/src/useCases/consumption/attributes/DeletePeerSharedAttributeAndNotifyOwner.ts Outdated Show resolved Hide resolved
...me/src/useCases/consumption/attributes/DeleteThirdPartyRelationshipAttributeAndNotifyPeer.ts Outdated Show resolved Hide resolved
packages/runtime/test/consumption/attributes.test.ts Outdated Show resolved Hide resolved
packages/runtime/test/consumption/attributes.test.ts Outdated Show resolved Hide resolved
packages/runtime/test/consumption/attributes.test.ts Outdated Show resolved Hide resolved
packages/runtime/test/consumption/attributes.test.ts Outdated Show resolved Hide resolved
packages/runtime/test/consumption/attributes.test.ts Outdated Show resolved Hide resolved
@jkoenig134 jkoenig134 added enhancement New feature or request bug Something isn't working and removed bug Something isn't working enhancement New feature or request labels Jan 31, 2025
@jkoenig134 jkoenig134 changed the title Return an error when deleting a SharedAttribute where the Relationship is still pending Deleting a SharedAttribute does not validate if it's possible to send the Notification Jan 31, 2025
@sebbi08 sebbi08 requested a review from jkoenig134 February 6, 2025 09:07
Milena-Czierlinski
Milena-Czierlinski reviewed Feb 7, 2025
View reviewed changes
Copy link
Contributor

@Milena-Czierlinski Milena-Czierlinski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some beauty comments :)

packages/runtime/src/useCases/consumption/attributes/DeletePeerSharedAttributeAndNotifyOwner.ts Outdated Show resolved Hide resolved
packages/runtime/src/useCases/consumption/attributes/DeletePeerSharedAttributeAndNotifyOwner.ts Outdated Show resolved Hide resolved
packages/runtime/src/useCases/consumption/attributes/DeletePeerSharedAttributeAndNotifyOwner.ts Outdated Show resolved Hide resolved
packages/runtime/src/useCases/consumption/attributes/DeletePeerSharedAttributeAndNotifyOwner.ts Outdated Show resolved Hide resolved
packages/runtime/test/lib/testUtils.ts Outdated Show resolved Hide resolved
packages/runtime/test/consumption/attributes.test.ts Outdated Show resolved Hide resolved
packages/runtime/test/consumption/attributes.test.ts Outdated Show resolved Hide resolved
packages/runtime/test/consumption/attributes.test.ts Outdated Show resolved Hide resolved
packages/runtime/test/consumption/attributes.test.ts Show resolved Hide resolved
packages/transport/src/modules/messages/MessageController.ts Outdated Show resolved Hide resolved
Milena-Czierlinski
Milena-Czierlinski approved these changes Feb 10, 2025
View reviewed changes
Copy link
Contributor

@Milena-Czierlinski Milena-Czierlinski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. @jkoenig134 @britsta Was there still anything open from your discussion?

@sebbi08 sebbi08 enabled auto-merge (squash) February 10, 2025 10:51
@sebbi08 sebbi08 merged commit b3a47c1 into main Feb 10, 2025
15 checks passed
@sebbi08 sebbi08 deleted the feature/ABL-354 branch February 10, 2025 10:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@britsta britsta britsta left review comments

@jkoenig134 jkoenig134 jkoenig134 approved these changes

@Milena-Czierlinski Milena-Czierlinski Milena-Czierlinski approved these changes

@Magnus-Kuhn Magnus-Kuhn Awaiting requested review from Magnus-Kuhn

Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sebbi08 @jkoenig134 @britsta @Milena-Czierlinski